Skip to content

relay-transforms: optionally use native gql fragment spreads for unmasked fragments #4975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

relay-transforms: optionally use native gql fragment spreads for unmasked fragments #4975

wants to merge 1 commit into from

Conversation

@sjbarag
Copy link
Contributor

@sjbarag sjbarag commented Apr 30, 2025

When Relay encountered a fragment spread with the @relay(mask: false) directive, it would previously inline that fragment text into the query itself. This is completely valid and equivalent to a native GraphQL fragment spread. That inlining can dramatically increase the size of serialize for a query with multiple fragments. Such repetition compresses extremely well, but clients and servers still pay the cost of having uncompressed strings in-memory (plus compress/decompress time).

Allow relay users to opt out of fragment inlining for @relay(mask: false) fragment spreads in queries, using a native GraphQL fragment spread. The reader AST and type generation are not affected so there's no impact to the relay runtime.

CC @Lalitha-Iyer

@sjbarag sjbarag force-pushed the dont-duplicate-unmasked-fragments-in-query branch from 3718890 to cdc91f3 Compare April 30, 2025 18:42
@sjbarag sjbarag marked this pull request as ready for review April 30, 2025 18:43
@evanyeung
Copy link
Contributor

evanyeung commented May 1, 2025

Hi @sjbarag, thanks for the PR. However, we generally discourage the use of @relay(mask: false) as an anti-pattern now as it breaks component encapsulation. Instead, a similar directive that fulfills the usual cases for @relay(mask: false) is @inline. Rather than updating the @relay directive, does this meet your needs?

@sjbarag
Copy link
Contributor Author

sjbarag commented May 2, 2025 •
edited
Loading

Hi @sjbarag, thanks for the PR. However, we generally discourage the use of @relay(mask: false) as an anti-pattern now as it breaks component encapsulation. Instead, a similar directive that fulfills the usual cases for @relay(mask: false) is @inline. Rather than updating the @relay directive, does this meet your needs?

Heyyy @evanyeung! Netflix has ~300 uses of @relay(mask: false) at the moment, so it's a pretty expensive migration. We'd like to get there, but it'll take quite a bit of time.

In the meantime though, this PR would reduce some of our query text strings by 93% (from 343_773 bytes to 22_078 bytes). That feels like a major win for anyone using @relay(mask: false) with minimal maintenance cost, even if it's discouraged.

@sjbarag sjbarag force-pushed the dont-duplicate-unmasked-fragments-in-query branch from cdc91f3 to b64c530 Compare May 7, 2025 20:51
@sjbarag
Copy link
Contributor Author

sjbarag commented May 15, 2025 •
edited
Loading

Hi @sjbarag, thanks for the PR. However, we generally discourage the use of @relay(mask: false) as an anti-pattern now as it breaks component encapsulation. Instead, a similar directive that fulfills the usual cases for @relay(mask: false) is @inline. Rather than updating the @relay directive, does this meet your needs?

Heyyy @evanyeung! Netflix has ~300 uses of @relay(mask: false) at the moment, so it's a pretty expensive migration. We'd like to get there, but it'll take quite a bit of time.

In the meantime though, this PR would reduce some of our query text strings by 93% (from 343_773 bytes to 22_078 bytes). That feels like a major win for anyone using @relay(mask: false) with minimal maintenance cost, even if it's discouraged.

(bump) Any further thoughts on whether this PR will be considered? 🙂

@sjbarag sjbarag force-pushed the dont-duplicate-unmasked-fragments-in-query branch 2 times, most recently from 83f91d5 to 338f995 Compare May 28, 2025 23:32
@sjbarag
relay-transforms: optionally use native gql fragment spreads for unma…
21ae65e 
…sked fragments

When Relay encountered a fragment spread with the `@relay(mask: false)`
directive, it would previously inline that fragment text into the query
itself. This is completely valid and equivalent to a native GraphQL
fragment spread. That inlining can dramatically increase the size of
serialize for a query with multiple fragments. Such repetition compresses
extremely well, but clients and servers still pay the cost of having
uncompressed strings in-memory (plus compress/decompress time).

Allow relay users to opt out of fragment inlining for
`@relay(mask: false)` fragment spreads in queries, using a native
GraphQL fragment spread. The reader AST and type generation are not
affected so there's no impact to the relay runtime.
@sjbarag sjbarag force-pushed the dont-duplicate-unmasked-fragments-in-query branch from 338f995 to 21ae65e Compare June 20, 2025 17:22
@sjbarag
Copy link
Contributor Author

sjbarag commented Jul 2, 2025

(one last bump) @captbaritone could you please weigh in on whether this is something Meta will consider merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed shared with relay team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sjbarag @evanyeung @facebook-github-bot